Feat/implement phase f#5
Conversation
Backup writes a dumps.Envelope through the checksum tee before the driver's native bytes, so the whole-file sha256 (and Verify's rehash) stays consistent. Restore strips and validates the envelope, handing the native-bytes body reader to the driver.
…cremental - Swap Sync's io.Pipe for jobs.Stream(64); use CloseErr to keep backup-failure propagation so a truncated dump is not committed. - Add SyncOpts.CrossEngine/Continuous; route cross-engine through a capability-gated runCrossEngineSync that returns a clear unsupported error (no driver advertises cross-engine yet). - Add sync --cross-engine/--continuous and backup --incremental/--base flags; continuous and incremental return honest unsupported errors. - Add Stream CloseErr propagation and clean-EOF unit tests.
|
Warning Review limit reached
More reviews will be available in 31 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughPhase F adds the core advanced-transfer machinery: a bounded ChangesPhase F Advanced Transfer
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Sync
participant jobs.Stream
participant BackupGoroutine
participant Restore
CLI->>Sync: Sync(opts)
Sync->>jobs.Stream: NewStream(64 chunks)
Sync->>BackupGoroutine: go conn.Backup(ctx, stream)
Sync->>Restore: conn.Restore(ctx, stream)
BackupGoroutine->>jobs.Stream: Write(chunks) — blocks when full
Restore->>jobs.Stream: Read(p) — drains chunks
alt Restore exits early
Restore-->>Sync: return err
Sync->>jobs.Stream: Close() — unblocks BackupGoroutine
BackupGoroutine->>jobs.Stream: Write → io.ErrClosedPipe
else Backup fails
BackupGoroutine->>jobs.Stream: CloseErr(bErr)
Restore->>jobs.Stream: Read → returns bErr after draining
end
sequenceDiagram
participant Restore as app.Restore
participant Catalog
participant DumpFile
participant Driver
Restore->>Catalog: ResolveChain(dumpID)
loop walk ParentID (cycle-detected)
Catalog-->>Restore: []Meta [base, inc1, inc2]
end
opt UpTo set
Restore->>Restore: truncateChain(chain, upTo)
end
loop each Meta in chain
Restore->>DumpFile: Open + ReadEnvelope
DumpFile-->>Restore: Envelope + body reader
Restore->>Driver: conn.Restore(opts{Clean: i==0}, body)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/sync.go (1)
97-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the real restore error when forcing stream shutdown.
On Line 98,
stream.Close()intentionally unblocks the backup writer; that can make backup returnio.ErrClosedPipe. On Lines 101-103, that inducedbackupErrtakes precedence and can mask the realrestoreErrfrom Line 97.Suggested fix
+import ( + "errors" + "io" +) ... restoreErr := dstConn.Restore(ctx, driver.RestoreOpts{TargetTables: opt.Tables, Clean: true}, stream) _ = stream.Close() // unblock the backup goroutine's Write immediately if Restore returned early backupErr := <-errCh + if restoreErr != nil { + if backupErr != nil && !errors.Is(backupErr, io.ErrClosedPipe) { + return errors.Join(restoreErr, backupErr) + } + return restoreErr + } if backupErr != nil { return backupErr } - return restoreErr + return nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/sync.go` around lines 97 - 104, The code on lines 97-104 has a logic issue where closing the stream to unblock the backup goroutine can cause it to return io.ErrClosedPipe, which then masks the actual restore error. Fix this by checking if restoreErr is not nil and returning it first (since that's the real operation error), and only return backupErr if restoreErr is nil. Alternatively, handle the specific case where backupErr is io.ErrClosedPipe (the expected error from intentionally closing the stream) and suppress it, allowing the real restoreErr to be returned instead.
🧹 Nitpick comments (3)
internal/driver/postgres/incremental_test.go (1)
14-51: 🧹 Nitpick | 🔵 TrivialAdd integration test for
BackupIncrementalto catchpg_receivewalinvocation regressions.Currently, no integration test exercises
BackupIncremental, so CLI contract changes in thepg_receivewalcommand invocation (args, env setup) will not be caught. The existingTestIncremental_SlotAndLSNCapturevalidates only slot lifecycle and base backup operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/driver/postgres/incremental_test.go` around lines 14 - 51, Add integration test coverage for the BackupIncremental method to catch pg_receivewal command invocation regressions. After the existing slot creation and base backup operations in TestIncremental_SlotAndLSNCapture (or in a new complementary test), invoke the BackupIncremental method on the *Conn object to exercise the WAL streaming via pg_receivewal command, capturing its output and verifying successful execution. This ensures that CLI contract changes in the pg_receivewal command arguments and environment setup will be detected during test execution.internal/app/cdc.go (2)
88-93: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse a per-sync state key instead of a global
cdcfile.Line 88 hardcodes
jobIDto"cdc", so different source/target continuous sync pairs will overwrite each other’s resume position file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/cdc.go` around lines 88 - 93, The jobID is hardcoded to the constant string "cdc" on line 88, causing all source/target sync pairs to share the same state file and overwrite each other's resume positions. Replace the hardcoded jobID with a dynamic value that incorporates both opt.From and opt.To (the source and target) to create a unique key for each distinct sync pair. This ensures each source/target combination maintains its own separate state file for proper resume functionality.
91-93: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winOnly ignore missing-state errors; surface other load failures.
Lines 91-93 currently ignore all
loadCDCStateerrors. A malformed or unreadable state file will silently reset progress instead of failing fast.Suggested fix
+import "errors" ... - if prev, err := loadCDCState(jobID); err == nil { - state = prev - } + if prev, err := loadCDCState(jobID); err == nil { + state = prev + } else if !errors.Is(err, os.ErrNotExist) { + return err + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/cdc.go` around lines 91 - 93, The error handling in the loadCDCState call currently ignores all errors when err is nil, which masks real failures like malformed or unreadable state files. Modify the error handling logic in the loadCDCState block to specifically check if the error is a missing-state error (typically a file-not-found type error) and only ignore that specific error type. For all other errors returned by loadCDCState, ensure they are surfaced by logging or returning them rather than silently continuing with the default state. This way, real state file corruption or access issues will be caught instead of silently resetting progress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/CROSS_ENGINE.md`:
- Around line 93-94: The documentation in CROSS_ENGINE.md at the specified lines
overstates the current index handling capabilities. The current implementation
only synthesizes CREATE TABLE IF NOT EXISTS statements and row inserts, but does
not actually recreate indexes. Reword the statement about indexes from claiming
they are created (but not optimized) to accurately reflect that index recreation
is not implemented yet, aligning the documentation with the actual behavior of
the canonical consume path.
In `@internal/app/canonical_consume.go`:
- Around line 19-21: The bufio.Scanner with a 16 MiB buffer limit can cause
token size violations when processing large rows, resulting in mid-stream aborts
and partial replay state. Replace the Scanner-based approach (where sc is
created with NewScanner and Buffer methods are called) with json.Decoder, which
does not impose token size limits on the input stream. This change should be
applied throughout the canonical_consume.go file, including all locations where
the Scanner is currently used for row streaming to ensure consistent behavior
and eliminate the risk of large-row transfer failures.
In `@internal/app/canonical_emit.go`:
- Around line 43-56: The scanned values in the vals slice can contain []byte
which gets marshaled as base64 JSON, but text/JSON/decimal-like values need to
be normalized first. In the loop where the map m is populated from t.Columns,
add normalization logic before assigning each value to m[c.Name] to detect and
transform any []byte values into their appropriate string or typed
representations based on the column type, ensuring that text/JSON/decimal values
are properly converted before being passed to writeJSONL.
In `@internal/app/restore.go`:
- Around line 63-74: The envelope returned from dumps.ReadEnvelope at line 63 is
being ignored (assigned to underscore), which means there is no validation that
the dump's driver matches the current driver before conn.Restore is called with
Clean=true on the first iteration. Capture the envelope instead of ignoring it,
extract the driver information from the envelope/meta, and add a strict driver
match validation before the conn.Restore call at line 74 to ensure the dump is
compatible with the current driver before any destructive operations like clean
are performed.
In `@internal/cli/backup.go`:
- Around line 31-48: The CLI currently accepts the --base flag without enforcing
that it can only be used with the --incremental flag, making the behavior
ambiguous when --base is provided without --incremental. Add a validation check
before or near the existing incremental backup validation that rejects the
combination when baseID is not empty and incremental is false. Return a
user-facing error that explains --base can only be used with --incremental,
similar to the structure of the existing incremental backup error that uses
errs.Error with Op, Code, Cause, and Hint fields.
In `@internal/driver/_mysqlcommon/incremental.go`:
- Around line 101-110: The binlogArgs function does not include SSL/TLS
configuration from the driver.Profile.SSLMode, which means incremental binlog
capture ignores required encryption policies. To fix this, modify the binlogArgs
function signature to accept the binlogBinary parameter (which indicates whether
the binary is mysqlbinlog or mariadb-binlog), create a helper function that maps
p.SSLMode to fork-specific SSL flags (MySQL uses --ssl-mode with
--ssl-ca/--ssl-cert/--ssl-key while MariaDB uses --ssl and
--ssl-verify-server-cert), and append the appropriate SSL flags to the argument
slice before adding the final since.File argument.
In `@internal/driver/postgres/incremental.go`:
- Around line 54-70: The incrementalArgs function contains two issues: the `-D`
argument uses `-` expecting stdout, but pg_receivewal requires an actual
directory path and does not support stdout streaming, so this argument must be
changed to specify a valid directory instead. Additionally, the comment on line
72 incorrectly describes `--no-loop` as controlling WAL segment termination when
it actually controls retry behavior on connection errors, so update the comment
to accurately reflect the flag's true purpose and remove the incorrect
characterization of its function.
In `@internal/jobs/stream_test.go`:
- Around line 177-190: The test TestStream_ConcurrentWriteCloseNoPanic spawns
three goroutines (for Close, Write, and Read operations) inside a loop without
waiting for them to complete, allowing the test to return before the concurrent
operations are actually executed and checked for race conditions. Add a
sync.WaitGroup to synchronize the spawned goroutines: initialize it before the
loop, call Add(3) for each iteration to account for the three goroutines, invoke
Done() at the end of each spawned goroutine function, and call Wait() after the
loop to ensure all goroutines complete before the test returns.
In `@internal/jobs/stream.go`:
- Around line 33-35: The Write method currently enqueues the entire input byte
slice p as a single chunk, which allows large writes to consume unbounded memory
and bypass the capChunks backpressure mechanism. Modify the Write method to
split the incoming p into appropriately sized chunks (respecting the intended
chunk size limit of ~1MB as mentioned in the NewStream documentation) and
enqueue each chunk individually. This ensures that large Write calls are
properly throttled and the buffer backpressure works as designed, rather than
allowing any single Write to circumvent the buffer bounds.
- Around line 72-73: The Read method of the Stream struct must handle
zero-length reads explicitly to comply with the io.Reader contract. Add a check
at the beginning of the Read method (before the overflow check) to return (0,
nil) immediately when len(p) == 0, ensuring that callers probing with empty
buffers do not block on the select statement that waits for data from s.ch or
s.done.
---
Outside diff comments:
In `@internal/app/sync.go`:
- Around line 97-104: The code on lines 97-104 has a logic issue where closing
the stream to unblock the backup goroutine can cause it to return
io.ErrClosedPipe, which then masks the actual restore error. Fix this by
checking if restoreErr is not nil and returning it first (since that's the real
operation error), and only return backupErr if restoreErr is nil. Alternatively,
handle the specific case where backupErr is io.ErrClosedPipe (the expected error
from intentionally closing the stream) and suppress it, allowing the real
restoreErr to be returned instead.
---
Nitpick comments:
In `@internal/app/cdc.go`:
- Around line 88-93: The jobID is hardcoded to the constant string "cdc" on line
88, causing all source/target sync pairs to share the same state file and
overwrite each other's resume positions. Replace the hardcoded jobID with a
dynamic value that incorporates both opt.From and opt.To (the source and target)
to create a unique key for each distinct sync pair. This ensures each
source/target combination maintains its own separate state file for proper
resume functionality.
- Around line 91-93: The error handling in the loadCDCState call currently
ignores all errors when err is nil, which masks real failures like malformed or
unreadable state files. Modify the error handling logic in the loadCDCState
block to specifically check if the error is a missing-state error (typically a
file-not-found type error) and only ignore that specific error type. For all
other errors returned by loadCDCState, ensure they are surfaced by logging or
returning them rather than silently continuing with the default state. This way,
real state file corruption or access issues will be caught instead of silently
resetting progress.
In `@internal/driver/postgres/incremental_test.go`:
- Around line 14-51: Add integration test coverage for the BackupIncremental
method to catch pg_receivewal command invocation regressions. After the existing
slot creation and base backup operations in TestIncremental_SlotAndLSNCapture
(or in a new complementary test), invoke the BackupIncremental method on the
*Conn object to exercise the WAL streaming via pg_receivewal command, capturing
its output and verifying successful execution. This ensures that CLI contract
changes in the pg_receivewal command arguments and environment setup will be
detected during test execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b28c1dc-3011-43cd-a6fd-dc800cb79f7d
📒 Files selected for processing (32)
CHANGELOG.mdREADME.mddocs/CDC.mddocs/CROSS_ENGINE.mddocs/INCREMENTAL.mdinternal/app/backup.gointernal/app/canonical.gointernal/app/canonical_consume.gointernal/app/canonical_emit.gointernal/app/canonical_test.gointernal/app/cdc.gointernal/app/cdc_test.gointernal/app/restore.gointernal/app/restore_chain_test.gointernal/app/sync.gointernal/app/sync_test.gointernal/cli/backup.gointernal/cli/restore.gointernal/cli/sync.gointernal/driver/_mysqlcommon/conn.gointernal/driver/_mysqlcommon/incremental.gointernal/driver/_mysqlcommon/incremental_test.gointernal/driver/mariadb/driver.gointernal/driver/mysql/driver.gointernal/driver/postgres/incremental.gointernal/driver/postgres/incremental_test.gointernal/dumps/chain.gointernal/dumps/chain_test.gointernal/dumps/envelope.gointernal/dumps/envelope_test.gointernal/jobs/stream.gointernal/jobs/stream_test.go
…s, incremental) - Rewrite Postgres BackupIncremental to write WAL to a temp dir with --endpos (pg_receivewal cannot stream to stdout); fix the misleading --no-loop comment (retry, not WAL-end). - Normalize []byte scanned values to string in canonical emit so they JSON round-trip as text instead of base64. - Replace bufio.Scanner with json.Decoder in ConsumeCanonical to remove the 16 MiB token cap that aborted large rows mid-replay. - Verify each chain dump's envelope driver matches the target before any destructive Clean in Restore (ErrIncompatibleEngine). - Pass fork-specific SSL flags to mysqlbinlog/mariadb-binlog from the profile SSLMode. - Split large Writes into <=1 MiB chunks so Stream's bounded channel bounds memory; return (0, nil) for zero-length Reads. - Join goroutines in the Stream race stress test with a WaitGroup. - Reject --base without --incremental in the backup command. - Correct the cross-engine doc: index recreation is not yet implemented (data + table structure only).
Phase F — Advanced Transfer (machinery; several CLI paths gated as honest follow-ups)
Lands the advanced-transfer machinery from spec §9. By design, the working user-facing pieces ship now; the paths that need a live replication-configured DB or driver-surface changes are gated with clear errors and documented as follow-ups (this box has no Docker, so those paths are compile-checked locally and validated by CI). README marks Phase F 🟡 Partial, not ✅.
✅ Works today (real, tested)
SIPHJSON header (type, base/parent IDs, WAL/binlog positions, checksum). Checksum covers envelope+body, soverifystays correct;restorestrips it before handing native bytes to the driver.restore <id>resolves the base→incremental chain and applies it in order;--up-to <id>stops early; cycle + broken-chain detection (termination proven).syncstreams through ajobs.Stream(default 64×1 MB) instead ofio.Pipe, exposing aFillPercentbackpressure metric and propagating a backup failure to the restore side viaCloseErr(no truncated dump committed as clean). Race-clean; the no-goroutine-leak invariant is mutation-tested.🟡 Machinery in place, CLI gated (follow-up wiring)
mysqlcommon.Conn).backup --incrementalreturns a clear "not yet wired" error — the end-to-end capture→envelope→catalog wiring + orphan-slot cleanup are tracked follow-ups.MapToNativetype matrix + JSONL emit/consume with per-engine identifier quoting and placeholders (unit-tested, incl. SQL-injection guards).sync --cross-engineis capability-gated off untildriver.Inspectcarries column types (typed schema introspection is the prerequisite).RunCDCis a polling scaffold (real logical-replication streaming viapglogrepl/binlog tailing is deferred). Gated off (no driver advertisesCapCDC); there is nosiphon cdccommand yet.Notable decisions / deviations from the original plan
jobs.Streamdesign — the plan'sClose()closed the buffer channel, which races a concurrentWrite(caught bygo test -race); the buffer channel is never closed (only adonesignal), making Close safe from any goroutine. AddedCloseErr(the bounded-buffer analogue ofio.PipeWriter.CloseWithError)."/`doubling) + per-engine placeholders ($nvs?), with values always parameterized. SQL extracted into pure builders + injection-guard tests.mysqlcommon.Conn(not per-packageConntypes, which don't exist post-Phase-E), parameterized by abinlogBinaryname.backup --incremental,sync --cross-engine,sync --continuousall return clearCodeUser/ErrDriverUnsupportederrors rather than silently no-op'ing or falsely succeeding.Verification
make tidyclean ·make test(14 packages, no failures) ·go test -race ./internal/app/... ./internal/jobs/...clean ·make lint0 (default +--build-tags integration) ·make buildok ·go build -tags integration ./...compiles.Follow-up gates (tracked) before incremental backup ships to users
pg_receivewal/mysqlbinlogstreaming invocations against livewal_level>=replica/binlog_format=ROWservers.driver.Inspect) to unlock cross-engine.See
docs/INCREMENTAL.md,docs/CROSS_ENGINE.md,docs/CDC.mdfor per-feature status.Summary by CodeRabbit
Release Notes
Documentation
New Features